ChannelManager read refactor follow ups by joostjager · Pull Request #4374 · lightningdevkit/rust-lightning

joostjager · 2026-02-03T13:05:35Z

Followup commits to improve the readability and maintainability of the ChannelManager deserialization code.

ldk-reviews-bot · 2026-02-03T13:05:38Z

👋 Thanks for assigning @valentinewallace as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

codecov · 2026-02-03T15:43:05Z

Codecov Report

❌ Patch coverage is 73.68421% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.02%. Comparing base (35ab03f) to head (b4429bb).
⚠️ Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	73.68%	13 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4374      +/-   ##
==========================================
+ Coverage   85.99%   86.02%   +0.02%     
==========================================
  Files         156      156              
  Lines      102766   102844      +78     
  Branches   102766   102844      +78     
==========================================
+ Hits        88378    88471      +93     
+ Misses      11879    11863      -16     
- Partials     2509     2510       +1

Flag	Coverage Δ
tests	`86.02% <73.68%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

valentinewallace

Care to throw in any of these mentioned updates? #4332 (comment) Seems like a good opportunity

valentinewallace · 2026-02-03T17:01:31Z

lightning/src/ln/channelmanager.rs

 			testing_dnssec_proof_offer_resolution_override: Mutex::new(new_hash_map()),
 		};

+		// Step 7: Replay pending claims and fail HTLCs.


In my experience using step numbers in ldk-sample main, it's kinda annoying to have to update all of them when something changes. Maybe that's not as much of a risk here

I thought it might help a bit with navigation in this huge function. In other languages one would quickly extract methods, but in my experience that isn't always in Rust. Would you prefer that? I can try it.

I wouldn't really prefer extracting methods. Just wanted to point it out, we can keep the numbers

Made it unnumbered headings

ldk-reviews-bot · 2026-02-03T17:02:08Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

joostjager · 2026-02-04T10:10:42Z

Care to throw in any of these mentioned updates? #4332 (comment) Seems like a good opportunity

Done

valentinewallace

Nice cleanups here!

valentinewallace · 2026-02-04T16:49:48Z

lightning/src/ln/channelmanager.rs

+		// consistency. Channels that are behind their monitors are force-closed. Channels without
+		// monitors (except those awaiting initial persistence) are rejected. Monitors without


I don't think it's true that channels without monitors will be rejected -- instead, we'll DecodeError::InvalidValue on the entire manager read. Were these comments added by Claude? I'm kinda not sold on them, they add to the review quite a bit and seem error prone/at risk of becoming out-of-date

Yes, clauded indeed. Unfortunate. First I went and corrected them, but I also felt what you mention about becoming out-of-date. In my opinion, there aren't enough comments in the code base, but comments that are located a bit further away from the code aren't ideal. Dropped the commit.

valentinewallace · 2026-02-04T16:51:15Z

lightning/src/ln/channelmanager.rs

 	async_receive_offer_cache: AsyncReceiveOfferCache,
+	// Marked `_legacy` because in versions > 0.2 we are taking steps to remove the requirement of
+	// regularly persisting the `ChannelManager` and instead rebuild the set of HTLC forwards from
+	// `Channel{Monitor}` data. See [`ChannelManager::read`].


I don't think ChannelManager::read adds anything to the understanding of what's going on here, can probably remove that sentence

Yes, this was pointing to the reconstruct boolean, but can't link that.

Removed now, show references will get to the usage sites quickly enough. Can't link in // comments anyway.

valentinewallace · 2026-02-04T17:03:39Z

lightning/src/ln/channelmanager.rs

+				.into_iter()
+				.zip(onion_fields)
+				.map(|((hash, htlcs), onion)| (hash, htlcs, onion))
+				.collect()


Hmm, we introduce an intermediate allocation here. Wonder if the whole creation of claimable_payments can move here, to avoid it? It feels somewhat fitting.

I've considered that, but it would need the keys for the legacy handling branch. But perhaps that's ok? The distinction between stage 1 and 2 isn't carved in stone.

Updated the commit with this. It's seem better indeed.

valentinewallace · 2026-02-04T17:10:17Z

lightning/src/ln/channelmanager.rs

 				.unwrap_or_else(new_hash_map),
 			inbound_payment_id_secret,
-			in_flight_monitor_updates,
+			in_flight_monitor_updates: in_flight_monitor_updates.unwrap_or_default(),


Will this use HashMap::new() or new_hash_map (which I think is what we want)? CI seems fine though...

I think it is the same?

HashMap::default() -> creates HashMap with RandomState::default() -> which calls RandomState::new() which is what new_hash_map does too?

If true, we could replace all new_hash_map calls with HashMap::default(), to make it look slightly less custom.

Reorder the struct fields to place all three `_legacy` fields together at the end, allowing the explanatory comment to appear once instead of being duplicated three times. Co-Authored-By: Claude Opus 4.5 <[email protected]>

Initialize pending_claiming_payments and monitor_update_blocked_actions_per_peer with None and resolve with unwrap_or_else, matching the pattern used for other optional hash map fields like pending_intercepted_htlcs_legacy. Co-Authored-By: Claude Opus 4.5 <[email protected]>

Since there is no semantic difference between None and Some(empty vec) for peer_storage_dir, simplify the type to Vec and use unwrap_or_default() when reading from TLV fields. Co-Authored-By: Claude Opus 4.5 <[email protected]>

Move the reconstruction of claimable_payments from the main ChannelManager::read into ChannelManagerData::read. This removes the separate claimable_htlc_purposes and claimable_htlc_onion_fields fields from ChannelManagerData, replacing them with the combined claimable_payments HashMap. This requires adding a node_signer parameter to ChannelManagerDataReadArgs to support verification of legacy hop data when reconstructing payment purposes for very old serialized data (pre-0.0.107). Co-Authored-By: Claude Opus 4.5 <[email protected]>

Since there is no semantic difference between None and Some(empty map) for in_flight_monitor_updates, simplify the type to HashMap and use unwrap_or_default() when reading from TLV fields. Co-Authored-By: Claude Opus 4.5 <[email protected]>

joostjager · 2026-02-05T09:43:44Z

Diff: https://github.com/lightningdevkit/rust-lightning/compare/b4429bb4f5bcab729e47028b1c4ce12b2960b136..b509c4a93d5cf46df24d1d1f20b5008741d61745

joostjager requested a review from valentinewallace February 3, 2026 13:25

joostjager marked this pull request as ready for review February 3, 2026 13:25

joostjager mentioned this pull request Feb 3, 2026

Split ChannelManager::read into two stages #4332

Merged

valentinewallace reviewed Feb 3, 2026

View reviewed changes

joostjager force-pushed the chanmgr-read-2 branch from 13a8a3f to b4429bb Compare February 4, 2026 10:08

joostjager requested a review from valentinewallace February 4, 2026 10:10

valentinewallace reviewed Feb 4, 2026

View reviewed changes

joostjager and others added 5 commits February 5, 2026 10:23

joostjager force-pushed the chanmgr-read-2 branch from b4429bb to b509c4a Compare February 5, 2026 09:40

joostjager requested a review from valentinewallace February 5, 2026 09:44

		// consistency. Channels that are behind their monitors are force-closed. Channels without
		// monitors (except those awaiting initial persistence) are rejected. Monitors without

Conversation

joostjager commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joostjager Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Feb 3, 2026

Uh oh!

joostjager commented Feb 4, 2026

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joostjager Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joostjager Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joostjager commented Feb 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

joostjager commented Feb 3, 2026 •

edited

Loading

ldk-reviews-bot commented Feb 3, 2026 •

edited

Loading

codecov bot commented Feb 3, 2026 •

edited

Loading

joostjager Feb 4, 2026 •

edited

Loading

joostjager Feb 5, 2026 •

edited

Loading

joostjager Feb 5, 2026 •

edited

Loading